Introduce github action for CI #572

mweinelt · 2020-11-15T02:59:02Z

It seemed to me like Travis-CI checks are not working anymore. I'm aware of the new pricing policy they introduced recently and suspected it might be due to that.

The CI last ran somewhere mid-october.

Since this project is hosted on GitHub, I believe their actions feature might be a good fit for the time being. So I started to port the travis tests to the best of my understanding. I hope that is alright.

You can look at the current state over here: https://github.com/mweinelt/TTS/actions/runs/363907718

There is currently the following issue, that was introduced in 39c71ee:

 ======================================================================
ERROR: Test if all layers are updated in a basic training cycle
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/TTS/TTS/tests/test_wavegrad_train.py", line 36, in test_train_step
    model.compute_noise_level(1000, 1e-6, 1e-2)
TypeError: compute_noise_level() takes 2 positional arguments but 4 were given

----------------------------------------------------------------------

I'll happyily rebase once this fix has hit the dev branch, so we can check if this works.

lexkoro · 2020-11-15T06:34:35Z

The Travis-CI checks itself are still running https://travis-ci.org/github/mozilla/TTS.
It just seems they are not integrated with github anymore, maybe because of the named new pricing policy.

erogol · 2020-11-29T18:50:57Z

CI is still running but not a part of Github anymore for a reason I don't know. So I close this.

Mic92 · 2020-12-06T08:48:46Z

CI is still running but not a part of Github anymore for a reason I don't know. So I close this.

I just saw this. The current UX of the CI is not great (https://github.com/mozilla/TTS/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc#issuecomment-739447469). What are the reasons to stick to travis?

erogol · 2020-12-07T09:24:57Z

There is no particular reason. I just wanted to keep it as it was working. But now I see it makes sense to move on.

erogol · 2020-12-07T09:41:43Z

@Mic92 can we merge this one upon your review? It looks simpler and just a single file change. Do you see any particular difference in comparison to your PR?

mweinelt · 2020-12-07T10:33:49Z

.github/workflows/test.yml

+          python3 setup.py egg_info
+          python3 -m pip install -e .
+      - name: Lint check
+        if: github.event_name == 'pull_request'


This is a direction translation of

TTS/.travis/script

Line 7 in 88bb20e

if [[ ( "$TRAVIS_PULL_REQUEST" != "false" ) && ( "$TEST_SUITE" == "lint" ) ]]; then

but I don't think limiting linting that way is smart.

I would also suggest:

Suggested change

if: github.event_name == 'pull_request'

because otherwise people doing pull requests might also have to fix linting errors that were introduced in the branches itself.

but if there is a linting error, it shouldn't pass the test anyway. So there should be no lining error in a branch that a PR is going on.

This assumes that the linter output is always the same. However new linter versions can bring new checks and errors

I am fine with applying these new checks and errors. Do you see any downside in that ?

Direct pushes to master/dev that contain linting errors will put the CI in an error state, but at least they will be visible. I don't see downsides that should prevent this.

mweinelt · 2020-12-07T10:35:24Z

Rebased and rescoped to Python 3.6-3.8. Also dropped Travis-CI.

Now the two pull requests should be more comparable.

Mic92 · 2020-12-07T10:48:33Z

This one also looks fine. I would still include https://github.com/Mic92/TTS/blob/ci/.github/dependabot.yml to automatically update github actions (usually fixes bit-rot) and also cherry-pick https://github.com/Mic92/TTS/blob/ci/pyproject.toml because this way one can do pip install -e and it would already install cython/numpy before loading the setup.py which requires those libraries.

erogol · 2020-12-07T10:55:18Z

This one also looks fine. I would still include https://github.com/Mic92/TTS/blob/ci/.github/dependabot.yml to automatically update github actions (usually fixes bit-rot) and also cherry-pick https://github.com/Mic92/TTS/blob/ci/pyproject.toml because this way one can do pip install -e and it would already install cython/numpy before loading the setup.py which requires those libraries.

what is the exact use case of pip install -e? I don't favor including a new file (meaning another maintenance point) if we can use requirements.txt.

Mic92 · 2020-12-07T10:59:28Z

This one also looks fine. I would still include https://github.com/Mic92/TTS/blob/ci/.github/dependabot.yml to automatically update github actions (usually fixes bit-rot) and also cherry-pick https://github.com/Mic92/TTS/blob/ci/pyproject.toml because this way one can do pip install -e and it would already install cython/numpy before loading the setup.py which requires those libraries.

what is the exact use case of pip install -e? I don't favor including a new file (meaning another maintenance point) if we can use requirements.txt.

The problem is that setup.py already imports numpy/cython and hence make it impossible to add a build time dependency on both. A different option would be to completely drop requirements.txt and only use pyproject.toml pypa/pip#8049 (comment) This would simplify both CI usage and what developers need to run to a single pip install -e ..

mweinelt · 2020-12-07T11:01:12Z

This one also looks fine. I would still include https://github.com/Mic92/TTS/blob/ci/.github/dependabot.yml to automatically update github actions (usually fixes bit-rot) and also cherry-pick https://github.com/Mic92/TTS/blob/ci/pyproject.toml because this way one can do pip install -e and it would already install cython/numpy before loading the setup.py which requires those libraries.

I'm not sure how the pyproject.toml and the requirements.txt would interact. Maybe you can expand on that?

FWIW: I do think that pyproject.toml is the superior format. Especially when relying on poetry, which I do strongly recommend using.

what is the exact use case of pip install -e? I don't favor including a new file (meaning another maintenance point) if we can use requirements.txt.

With pip install -e . the local package will be installed in an editable fashion, as in: not as an egg. Not sure that's actually required for anything.

erogol · 2020-12-07T11:21:02Z

for pip install -e one can use python setup.py develop I guess until we have an official release. Let me know if there is a particular difference.

But I see the point of using pyproject.toml. Do you know any other way to indicate setup dependencies?

mweinelt · 2020-12-07T11:23:16Z

The reason why @Mic92 introduced the pyproject.toml is that it provides build-time dependency support. This is required because numpy needs to be preinstalled as it is directly used in setup.py.

TTS/setup.py

Line 8 in 88bb20e

import numpy

mweinelt · 2020-12-07T11:24:36Z

But I see the point of using pyproject.toml. Do you know any other way to indicate setup dependencies?

I don't think this can be done by using a single requirements.txt, if that is what you are asking.

Mic92 · 2020-12-07T11:26:08Z

for pip install -e one can use python setup.py develop I guess until we have an official release. Let me know if there is a particular difference.

But I see the point of using pyproject.toml. Do you know any other way to indicate setup dependencies?

Calling setup.py directly has the same issue as pip install -e .. They both need cython/numpy beeing manually installed or the build will fail. This is not optimal for people that just want to run:

$ pip install git+https://github.com/mozilla/TTS

erogol · 2020-12-07T11:27:01Z

But I see the point of using pyproject.toml. Do you know any other way to indicate setup dependencies?

I don't think this can be done by using a single requirements.txt, if that is what you are asking.

I meant in general is there any other way to define setup.py deps than pyproject.toml?

If you think .toml is the best way then I think we can include it as it is useful for CI and first-time installs as you've indicated.

mweinelt · 2020-12-07T11:32:01Z

But I see the point of using pyproject.toml. Do you know any other way to indicate setup dependencies?

I don't think this can be done by using a single requirements.txt, if that is what you are asking.

I meant in general is there any other way to define setup.py deps than pyproject.toml?

According to pypa/pip#2381 (comment) pyproject.toml is the way to go.

Mic92 · 2020-12-07T11:35:22Z

But I see the point of using pyproject.toml. Do you know any other way to indicate setup dependencies?

I don't think this can be done by using a single requirements.txt, if that is what you are asking.

I meant in general is there any other way to define setup.py deps than pyproject.toml?

Not as far as I know: pypa/pip#2381
There is setup_requires but it does not work in the way one expect and cannot be used to install build time dependencies. This also a big pain point we have in NixOS/nixpkgs when packaging python.
The escape hatch would be poetry, which is closer to modern package manager like cargo or yarn and capture the whole dependency tree.

mweinelt · 2020-12-07T11:43:13Z

Updated CI action can be seen here https://github.com/mweinelt/TTS/runs/1510390094.

Python 3.6 fails because recent numpy versions require Python 3.7, see https://github.com/numpy/numpy/blob/a3c09377e80a37deedc486ec30a7e852b7a8e564/setup.py#L475.

https://numpy.org/devdocs/release/1.20.0-notes.html

Mic92 · 2020-12-07T11:46:33Z

@mweinelt I also noticed this here: #591 (comment)

mweinelt · 2020-12-07T12:02:23Z

I tried pinning numpy<1.20 and it installs that just fine. Then pyworld comes along and then there isnumpy==1.20-rc1 for some odd reason.

https://github.com/mweinelt/TTS/runs/1510479798?check_suite_focus=true

mweinelt · 2020-12-07T12:05:24Z

I think considering Debian Stable already has Python 3.7 dropping support for Python 3.6 wouldn't be a big loss.

.github/workflows/test.yml

erogol · 2020-12-07T14:31:51Z

.github/workflows/test.yml

+          python3 setup.py egg_info
+          python3 -m pip install -e .
+      - name: Lint check
+        if: github.event_name == 'pull_request'


I am fine with applying these new checks and errors. Do you see any downside in that ?

tests/test_server_package.sh

erogol · 2020-12-07T14:36:01Z

I think considering Debian Stable already has Python 3.7 dropping support for Python 3.6 wouldn't be a big loss.

I'd prefer to keep supporting 3.6

erogol · 2020-12-07T14:42:43Z

I tried pinning numpy<1.20 and it installs that just fine. Then pyworld comes along and then there isnumpy==1.20-rc1 for some odd reason.

https://github.com/mweinelt/TTS/runs/1510479798?check_suite_focus=true

I don't see why it forces to install numpy 1.20

Reading setup.py will import numpy already, hence pip has no way to figure out what dependencies are needed.

mweinelt · 2020-12-07T15:55:46Z

I tried pinning numpy<1.20 and it installs that just fine. Then pyworld comes along and then there isnumpy==1.20-rc1 for some odd reason.
https://github.com/mweinelt/TTS/runs/1510479798?check_suite_focus=true

I don't see why it forces to install numpy 1.20

Opened up a bug report over at pypa/pip#9239 including a minimal reproducer.

Also opened up an issue over at pyworld, so maybe they can release a version with a proper environment pin, thereby resolving the situation. JeremyCCHsu/Python-Wrapper-for-World-Vocoder#59

mweinelt · 2020-12-07T16:50:23Z

Explanation of whats happening: pypa/pip#9239 (comment).

I guess this broke, when numpy released the first version incompatible with Python 3.6. So we currently have no way until pyworld pushes a fix. I opened JeremyCCHsu/Python-Wrapper-for-World-Vocoder#59 to get that process started.

Meanwhile I fixed up the lint step and a few issues have accumulated. https://github.com/mweinelt/TTS/runs/1511860335?check_suite_focus=true

Numpy starting with 1.20 has dropped support for Python 3.6.

Mic92 · 2020-12-08T09:56:39Z

Travis is no longer free btw: https://news.ycombinator.com/item?id=25338983

erogol · 2020-12-08T10:05:05Z

I think it is ready to merge than give that travis is not an option any more.

Anything else before merge ?

erogol · 2020-12-08T10:22:22Z

I just realized that Github Actions are not allowed by Mozilla. Now I try to find a workaround.

erogol · 2020-12-08T10:23:40Z

or do you have any other suggestions as a CI alternative for open-source?

erogol · 2020-12-08T10:39:30Z

I guess I go for Circle CI

mweinelt · 2020-12-08T17:51:34Z

I just realized that Github Actions are not allowed by Mozilla. Now I try to find a workaround.

Ah, meh.

or do you have any other suggestions as a CI alternative for open-source?

Not really.

I hope once the issues with pyworld are fixed we can integrate the pyproject.toml changes.

erogol · 2020-12-08T23:10:54Z

Sorry for being unaware of it

feel free to introduce pyproject.toml once it is the right time.

Forcing do_trim_silence to False in the extract TTS script

erogol closed this Nov 29, 2020

erogol reopened this Dec 7, 2020

mweinelt commented Dec 7, 2020

View reviewed changes

Mic92 mentioned this pull request Dec 7, 2020

Migrate to github actions #591

Closed

erogol reviewed Dec 7, 2020

View reviewed changes

specify build dependencies

c57fd73

Reading setup.py will import numpy already, hence pip has no way to figure out what dependencies are needed.

mweinelt added 2 commits December 7, 2020 20:53

Migrate to github actions for CI tests

857172b

Pin numpy<1.20 for continued python3.6 support

128fd94

Numpy starting with 1.20 has dropped support for Python 3.6.

erogol merged commit 2f689cb into mozilla:dev Dec 8, 2020

erogol mentioned this pull request Dec 8, 2020

Revert "Introduce github action for CI" #593

Merged

mweinelt deleted the gh-actions branch December 8, 2020 23:13

Mic92 pushed a commit to Mic92/TTS that referenced this pull request Oct 27, 2021

Merge pull request mozilla#572 from a-froghyar/fix-extract-tts-script

79241b6

Forcing do_trim_silence to False in the extract TTS script

Introduce github action for CI #572

Introduce github action for CI #572

Conversation

mweinelt commented Nov 15, 2020 • edited Loading

lexkoro commented Nov 15, 2020

erogol commented Nov 29, 2020

Mic92 commented Dec 6, 2020 • edited Loading

erogol commented Dec 7, 2020

erogol commented Dec 7, 2020

mweinelt Dec 7, 2020

Choose a reason for hiding this comment

Mic92 Dec 7, 2020

Choose a reason for hiding this comment

erogol Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

Mic92 Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

erogol Dec 7, 2020

Choose a reason for hiding this comment

mweinelt Dec 7, 2020

Choose a reason for hiding this comment

mweinelt commented Dec 7, 2020 • edited Loading

Mic92 commented Dec 7, 2020

erogol commented Dec 7, 2020

Mic92 commented Dec 7, 2020 • edited Loading

mweinelt commented Dec 7, 2020 • edited Loading

erogol commented Dec 7, 2020 • edited Loading

mweinelt commented Dec 7, 2020

mweinelt commented Dec 7, 2020

Mic92 commented Dec 7, 2020

erogol commented Dec 7, 2020 • edited Loading

mweinelt commented Dec 7, 2020

Mic92 commented Dec 7, 2020 • edited Loading

mweinelt commented Dec 7, 2020 • edited Loading

Mic92 commented Dec 7, 2020

mweinelt commented Dec 7, 2020 • edited Loading

mweinelt commented Dec 7, 2020

erogol Dec 7, 2020

Choose a reason for hiding this comment

erogol commented Dec 7, 2020

erogol commented Dec 7, 2020

mweinelt commented Dec 7, 2020 • edited Loading

mweinelt commented Dec 7, 2020

Mic92 commented Dec 8, 2020

erogol commented Dec 8, 2020

erogol commented Dec 8, 2020

erogol commented Dec 8, 2020

erogol commented Dec 8, 2020

mweinelt commented Dec 8, 2020

erogol commented Dec 8, 2020

mweinelt commented Nov 15, 2020 •

edited

Loading

Mic92 commented Dec 6, 2020 •

edited

Loading

erogol Dec 7, 2020 •

edited

Loading

Mic92 Dec 7, 2020 •

edited

Loading

mweinelt commented Dec 7, 2020 •

edited

Loading

Mic92 commented Dec 7, 2020 •

edited

Loading

mweinelt commented Dec 7, 2020 •

edited

Loading

erogol commented Dec 7, 2020 •

edited

Loading

erogol commented Dec 7, 2020 •

edited

Loading

Mic92 commented Dec 7, 2020 •

edited

Loading

mweinelt commented Dec 7, 2020 •

edited

Loading

mweinelt commented Dec 7, 2020 •

edited

Loading

mweinelt commented Dec 7, 2020 •

edited

Loading